Fixes to the xenoprofile Linux driver.

The active_domains code has race conditions:

* oprofile_set_active() calls set_active() method without holding
  start_sem.  This is clearly wrong, as xenoprof_set_active() makes
  several hypercalls.  oprofile_start(), for instance, could run in
  the middle of xenoprof_set_active().

* adomain_write(), adomain_read() and xenoprof_set_active() access
  global active_domains[] and adomains without synchronization.  I
  went for a simple, obvious fix and created another mutex.  Instead,
  one could move the shared data into oprof.c and protect it with
  start_sem, but that's more invasive.

Also clean up the code dealing with /dev/oprofile/active_domains:

* Use parameters instead of global variables to pass domain ids
  around.  Give those globals internal linkage.

* Allocate buffers dynamically to conserve stack space.

* Treat writes with size zero exactly like a write containing no
  domain id.  Before, zero-sized write was ignored, which is not the
  same.

* Parse domain ids as unsigned numbers.  Before, the first one was
  parsed as signed number.

  Because ispunct()-punctuation is ignored between domain ids, signs
  are still silently ignored except for the first number.  Hmm.

* Make parser accept whitespace as domain separator, because that's
  what you get when reading the file.

* EINVAL on domain ids overflowing domid_t.  Before, they were
  silently truncated.

* EINVAL on too many domain ids.  Before, the excess ones were
  silently ignored.

* Reset active domains on failure halfway through setting them.

* Fix potential buffer overflow in adomain_read().  Couldn't really
  happen because buffer was sufficient for current value of
  MAX_OPROF_DOMAINS.

Signed-off-by: Markus Armbruster <armbru@redhat.com>

diff --git a/linux-2.6-xen-sparse/arch/i386/oprofile/xenoprof.c b/linux-2.6-xen-sparse/arch/i386/oprofile/xenoprof.c
index 5365ed6fccaa3129304c4afdf74555e4569de226..80323cd2d134d7067791d4ff9c3f9d6774b36351 100644 (file)
--- a/linux-2.6-xen-sparse/arch/i386/oprofile/xenoprof.c
+++ b/linux-2.6-xen-sparse/arch/i386/oprofile/xenoprof.c
@@ -289,9 +289,13 @@ static int xenoprof_set_active(int * active_domains,
 
        for (i=0; i<adomains; i++) {
                domid = active_domains[i];
+               if (domid != active_domains[i]) {
+                       ret = -EINVAL;
+                       goto out;
+               }
                ret = HYPERVISOR_xenoprof_op(XENOPROF_set_active, &domid);
                if (ret)
-                       return (ret);
+                       goto out;
                if (active_domains[i] == 0)
                        set_dom0 = 1;
        }
@@ -300,8 +304,11 @@ static int xenoprof_set_active(int * active_domains,
                domid = 0;
                ret = HYPERVISOR_xenoprof_op(XENOPROF_set_active, &domid);
        }
-       
-       active_defined = 1;
+
+out:
+       if (ret)
+               HYPERVISOR_xenoprof_op(XENOPROF_reset_active_list, NULL);
+       active_defined = !ret;
        return ret;
 }
 

diff --git a/patches/linux-2.6.16.13/xenoprof-generic.patch b/patches/linux-2.6.16.13/xenoprof-generic.patch
index 61a7d33d60de7e6bbbf236fd4e8b37d0d1ac864c..b54c6a6d4f2764de4d6e500b142bf362bf95b75e 100644 (file)
--- a/patches/linux-2.6.16.13/xenoprof-generic.patch
+++ b/patches/linux-2.6.16.13/xenoprof-generic.patch
@@ -225,19 +225,21 @@ diff -pruN ../pristine-linux-2.6.16.13/drivers/oprofile/oprof.c ./drivers/oprofi
  struct oprofile_operations oprofile_ops;
  
  unsigned long oprofile_started;
-@@ -33,6 +37,17 @@ static DECLARE_MUTEX(start_sem);
+@@ -33,6 +37,19 @@ static DECLARE_MUTEX(start_sem);
   */
  static int timer = 0;
  
-+extern unsigned int adomains;
-+extern int active_domains[MAX_OPROF_DOMAINS];
-+
-+int oprofile_set_active(void)
++int oprofile_set_active(int active_domains[], unsigned int adomains)
 +{
-+      if (oprofile_ops.set_active)
-+              return oprofile_ops.set_active(active_domains, adomains);
++      int err;
++
++      if (!oprofile_ops.set_active)
++              return -EINVAL;
 +
-+      return -EINVAL;
++      down(&start_sem);
++      err = oprofile_ops.set_active(active_domains, adomains);
++      up(&start_sem);
++      return err;
 +}
 +
  int oprofile_setup(void)
@@ -251,7 +253,7 @@ diff -pruN ../pristine-linux-2.6.16.13/drivers/oprofile/oprof.h ./drivers/oprofi
  
  int oprofile_set_backtrace(unsigned long depth);
 +
-+int oprofile_set_active(void);
++int oprofile_set_active(int active_domains[], unsigned int adomains);
   
  #endif /* OPROF_H */
 diff -pruN ../pristine-linux-2.6.16.13/drivers/oprofile/oprofile_files.c ./drivers/oprofile/oprofile_files.c
@@ -280,7 +282,7 @@ diff -pruN ../pristine-linux-2.6.16.13/drivers/oprofile/oprofile_files.c ./drive
  unsigned long fs_buffer_size = 131072;
  unsigned long fs_cpu_buffer_size = 8192;
  unsigned long fs_buffer_watershed = 32768; /* FIXME: tune */
-@@ -117,11 +123,79 @@ static ssize_t dump_write(struct file * 
+@@ -117,11 +123,108 @@ static ssize_t dump_write(struct file * 
  static struct file_operations dump_fops = {
        .write          = dump_write,
  };
@@ -288,63 +290,92 @@ diff -pruN ../pristine-linux-2.6.16.13/drivers/oprofile/oprofile_files.c ./drive
 +
 +#define TMPBUFSIZE 512
 +
-+unsigned int adomains = 0;
-+long active_domains[MAX_OPROF_DOMAINS];
++static unsigned int adomains = 0;
++static int active_domains[MAX_OPROF_DOMAINS + 1];
++static DEFINE_MUTEX(adom_mutex);
 +
 +static ssize_t adomain_write(struct file * file, char const __user * buf, 
 +                           size_t count, loff_t * offset)
 +{
-+      char tmpbuf[TMPBUFSIZE];
-+      char * startp = tmpbuf;
-+      char * endp = tmpbuf;
++      char *tmpbuf;
++      char *startp, *endp;
 +      int i;
 +      unsigned long val;
++      ssize_t retval = count;
 +      
 +      if (*offset)
 +              return -EINVAL; 
-+      if (!count)
-+              return 0;
 +      if (count > TMPBUFSIZE - 1)
 +              return -EINVAL;
 +
-+      memset(tmpbuf, 0x0, TMPBUFSIZE);
++      if (!(tmpbuf = kmalloc(TMPBUFSIZE, GFP_KERNEL)))
++              return -ENOMEM;
 +
-+      if (copy_from_user(tmpbuf, buf, count))
++      if (copy_from_user(tmpbuf, buf, count)) {
++              kfree(tmpbuf);
 +              return -EFAULT;
-+      
-+      for (i = 0; i < MAX_OPROF_DOMAINS; i++)
-+              active_domains[i] = -1;
-+      adomains = 0;
++      }
++      tmpbuf[count] = 0;
 +
-+      while (1) {
-+              val = simple_strtol(startp, &endp, 0);
++      mutex_lock(&adom_mutex);
++
++      startp = tmpbuf;
++      /* Parse one more than MAX_OPROF_DOMAINS, for easy error checking */
++      for (i = 0; i <= MAX_OPROF_DOMAINS; i++) {
++              val = simple_strtoul(startp, &endp, 0);
 +              if (endp == startp)
 +                      break;
-+              while (ispunct(*endp))
++              while (ispunct(*endp) || isspace(*endp))
 +                      endp++;
-+              active_domains[adomains++] = val;
-+              if (adomains >= MAX_OPROF_DOMAINS)
-+                      break;
++              active_domains[i] = val;
++              if (active_domains[i] != val)
++                      /* Overflow, force error below */
++                      i = MAX_OPROF_DOMAINS + 1;
 +              startp = endp;
 +      }
-+      if (oprofile_set_active())
-+              return -EINVAL; 
-+      return count;
++      /* Force error on trailing junk */
++      adomains = *startp ? MAX_OPROF_DOMAINS + 1 : i;
++
++      kfree(tmpbuf);
++
++      if (adomains > MAX_OPROF_DOMAINS
++          || oprofile_set_active(active_domains, adomains)) {
++              adomains = 0;
++              retval = -EINVAL;
++      }
++
++      mutex_unlock(&adom_mutex);
++      return retval;
 +}
 +
 +static ssize_t adomain_read(struct file * file, char __user * buf, 
 +                          size_t count, loff_t * offset)
 +{
-+      char tmpbuf[TMPBUFSIZE];
-+      size_t len = 0;
++      char * tmpbuf;
++      size_t len;
 +      int i;
-+      /* This is all screwed up if we run out of space */
-+      for (i = 0; i < adomains; i++) 
-+              len += snprintf(tmpbuf + len, TMPBUFSIZE - len, 
-+                              "%u ", (unsigned int)active_domains[i]);
-+      len += snprintf(tmpbuf + len, TMPBUFSIZE - len, "\n");
-+      return simple_read_from_buffer((void __user *)buf, count, 
-+                                     offset, tmpbuf, len);
++      ssize_t retval;
++
++      if (!(tmpbuf = kmalloc(TMPBUFSIZE, GFP_KERNEL)))
++              return -ENOMEM;
++
++      mutex_lock(&adom_mutex);
++
++      len = 0;
++      for (i = 0; i < adomains; i++)
++              len += snprintf(tmpbuf + len,
++                              len < TMPBUFSIZE ? TMPBUFSIZE - len : 0,
++                              "%u ", active_domains[i]);
++      WARN_ON(len > TMPBUFSIZE);
++      if (len != 0 && len <= TMPBUFSIZE)
++              tmpbuf[len-1] = '\n';
++
++      mutex_unlock(&adom_mutex);
++
++      retval = simple_read_from_buffer(buf, count, offset, tmpbuf, len);
++
++      kfree(tmpbuf);
++      return retval;
 +}
 +
 +